Allow enabling `config-include` feature in config #14196

lschuermann · 2024-07-05T18:59:41Z

What does this PR try to resolve?

Prior to this change, it is not possible to enable the unstable config-include flag from within a config file itself. This is due to the fact that, while cargo does reload configuration files if this flag is set, it does not parse the top-level configuration file's unstable flags before checking whether this flag is present.

This commit forces cargo to load unstable features before this check, and if the flag is present, re-loads configuration files with config-include enabled.

Addresses #7723 (comment).

How should we test and review this PR?

This PR adds a testcase for this behavior to cargo's testsuite. In general, it can be replicated with a config similar to that in the testcase, or as illustrated in #7723 (comment).

Additional information

I'm not sure whether this is the right(TM) fix, or whether the second self.load_unstable_flags_from_config()?; at the bottom of configure() would still be required. I have a feeling it might not be, as reload_rooted_at also calls out to load_unstable_flags_from_config, to collect flags from any newly included configs. If that is not called, no other file was loaded, and thus there should not be a need to re-load unstable flags.

Resolved: #14196 (comment)

rustbot · 2024-07-05T18:59:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

epage · 2024-07-05T19:07:35Z

r? weihanglo

If thats ok with you as you have a lot more context on this feature.

weihanglo

Thanks for the contribution!

Could you split this into two commits that the first one includes the new test, which demonstrates the buggy behavior and passes the CI pipeline. The second commit fixes both the behavior and the test. This pattern is documented here as a tip, which helps use make sure we're not fixing a nonexistent bug. The first commit is also served as an MRE.

weihanglo · 2024-07-05T23:38:51Z

src/cargo/util/context/mod.rs

+        // Load the unstable flags from config file here first, as the config
+        // file itself may enable inclusion of other configs. In that case, we
+        // want to re-load configs with includes enabled:
+        self.load_unstable_flags_from_config()?;


I think this is correct, and it should be safe to remove the other call of load_unstable_flags_from_config at the bottom of this function.

Removed, thanks for the confirmation!

weihanglo · 2024-07-06T00:00:53Z

tests/testsuite/config_include.rs

+    assert_eq!(gctx.get::<i32>("key2").unwrap(), 2);
+    assert_eq!(gctx.get::<i32>("key3").unwrap(), 4);
+}
+


Could you add another test to ensure that

A mix of hierarchy and include works correctly.

Fields under the [unstable] table are merged correctly.

#[cargo_test] fn mix_of_hierarchy_and_include() { write_config_at( "foo/.cargo/config.toml", " include = 'other.toml' key1 = 1 # also make sure unstable flags merge in the correct order [unstable] features = ['1'] ", ); write_config_at( "foo/.cargo/other.toml", " key1 = 2 key2 = 2 [unstable] features = ['2'] ", ); write_config_at( ".cargo/config.toml", " include = 'other.toml' key1 = 3 key2 = 3 key3 = 3 [unstable] features = ['3'] ", ); write_config_at( ".cargo/other.toml", " key1 = 4 key2 = 4 key3 = 4 key4 = 4 [unstable] features = ['4'] ", ); let gctx = GlobalContextBuilder::new() .unstable_flag("config-include") .cwd("foo") .nightly_features_allowed(true) .build(); assert_eq!(gctx.get::<i32>("key1").unwrap(), 1); assert_eq!(gctx.get::<i32>("key2").unwrap(), 2); assert_eq!(gctx.get::<i32>("key3").unwrap(), 3); assert_eq!(gctx.get::<i32>("key4").unwrap(), 4); assert_eq!( gctx.get::<Vec<String>>("unstable.features").unwrap(), vec![ "4".to_string(), "3".to_string(), "2".to_string(), "1".to_string() ] ); }

Thank you for this detailed example!

If I'm not mistaken, this example is not currently present in the testsuite and seems useful on its own, even when just enabling config include as an unstable CLI flag. Given the tricky code around re-loading the config in case of feature changes, would it make sense to include your test-case both on its own verbatim, and with config-include feature? That would make sure that cargo shows identical behavior for both methods of enabling this feature.

In that case I'd add this test case twice, one verbatim as you provided and one as a _with_enable_in_unstable_config variant. I'll push this in a second, let me know whether this makes sense to you.

Yeah. That sounds good to me :)

Co-authored-by: Weihang Lo <me@weihanglo.tw>

Prior to this change, it is not possible to enable the unstable `config-include` flag from within a config file itself. This is due to the fact that, while cargo does reload configuration files if this flag is set, it does not parse the top-level configuration file's unstable flags before checking whether this flag is present. This commit forces cargo to load unstable features before this check, and if the flag is present, re-loads configuration files with `config-include` enabled.

lschuermann · 2024-07-06T00:40:13Z

@rustbot review

lschuermann · 2024-07-06T00:49:58Z

Could you split this into two commits that the first one includes the new test, which demonstrates the buggy behavior and passes the CI pipeline. The second commit fixes both the behavior and the test. This pattern is documented here as a tip, which helps use make sure we're not fixing a nonexistent bug. The first commit is also served as an MRE.

I don't know whether there's any infrastructure in place to automate this check (other than pushing every commit individually), but all commits pass the respective tests for me locally:

> git rebase -i --exec 'git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include' HEAD~3
hint: Waiting for your editor to close the file... Waiting for Emacs...
Executing: git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include
 8bfc31f97 test(add): show config-include cannot be enabled in top-level config

running 13 tests
.............
test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 3353 filtered out; finished in 0.31s

Executing: git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include
 45e5cb496 test(add): show config-include correctly merges unstable configs

running 15 tests
...............
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 3353 filtered out; finished in 0.35s

Executing: git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include
 0e35bea6c Allow enabling `config-include` feature in config

running 15 tests
...............
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 3353 filtered out; finished in 0.33s

Successfully rebased and updated refs/heads/dev/config-include-enable-in-config.

weihanglo

Awesome. Let's merge it!

weihanglo · 2024-07-06T01:01:48Z

I don't know whether there's any infrastructure in place to automate this check (other than pushing every commit individually), but all commits pass the respective tests for me locally:

Not really. CI doesn't enforce that. It's a good idea if we make it an optional CI job, though PR with lots of commits might not like it 😆.

weihanglo · 2024-07-06T01:01:53Z

@bors r+

bors · 2024-07-06T01:01:55Z

📌 Commit 0e35bea has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-07-06T01:03:02Z

⌛ Testing commit 0e35bea with merge c7be9e4...

bors · 2024-07-06T01:43:19Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing c7be9e4 to master...

Update cargo 20 commits in a515d463427b3912ec0365d106791f88c1c14e1b..154fdac39ae9629954e19e9986fd2cf2cdd8d964 2024-07-02 20:53:36 +0000 to 2024-07-07 01:28:23 +0000 - test: relax redactions for rust-lang/rust (rust-lang/cargo#14203) - use "bootstrap" instead of "rustbuild" (rust-lang/cargo#14207) - test: migrate serveral files to snapbox (rust-lang/cargo#14180) - Add rustdocflags to Unit's Debug impl (rust-lang/cargo#14201) - Allow enabling `config-include` feature in config (rust-lang/cargo#14196) - fix(test): Restore `does_not_contain` for check (rust-lang/cargo#14198) - test: migrate patch, pkgid, proc_macro and progress to snapbox (rust-lang/cargo#14181) - test: Migrate jobserver to snapbox (rust-lang/cargo#14191) - chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186) - test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193) - test: migrate cfg and check to snapbox (rust-lang/cargo#14185) - test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170) - Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900) - test: Migrate network tests to snapbox (rust-lang/cargo#14187) - test: migrate some files to snapbox (rust-lang/cargo#14113) - test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161) - chore: fix some typos (rust-lang/cargo#14182) - fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026) - test：migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171) - test: Migrate git_auth to snapbox (rust-lang/cargo#14172)

rustbot assigned epage Jul 5, 2024

rustbot added A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2024

lschuermann mentioned this pull request Jul 5, 2024

Compile without Make / migrate flags to .cargo/config.toml tock/tock#4054

Closed

2 tasks

rustbot assigned weihanglo and unassigned epage Jul 5, 2024

weihanglo requested changes Jul 6, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024

lschuermann and others added 3 commits July 5, 2024 20:37

test(add): show config-include cannot be enabled in top-level config

8bfc31f

test(add): show config-include correctly merges unstable configs

45e5cb4

Co-authored-by: Weihang Lo <me@weihanglo.tw>

lschuermann force-pushed the dev/config-include-enable-in-config branch from 0b35010 to 0e35bea Compare July 6, 2024 00:39

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jul 6, 2024

weihanglo approved these changes Jul 6, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024

bors merged commit c7be9e4 into rust-lang:master Jul 6, 2024
22 checks passed

weihanglo mentioned this pull request Jul 7, 2024

Update cargo rust-lang/rust#127385

Merged

rustbot added this to the 1.81.0 milestone Jul 7, 2024

lschuermann mentioned this pull request Jul 8, 2024

Update Rust nightly toolchain to nightly-2024-07-08 tock/tock#4071

Merged

2 tasks

bradjc mentioned this pull request Jul 9, 2024

Tracking issue for config-include #7723

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow enabling `config-include` feature in config #14196

Allow enabling `config-include` feature in config #14196

lschuermann commented Jul 5, 2024 •

edited

Loading

rustbot commented Jul 5, 2024

epage commented Jul 5, 2024

weihanglo left a comment

weihanglo Jul 5, 2024

lschuermann Jul 6, 2024

weihanglo Jul 6, 2024 •

edited

Loading

lschuermann Jul 6, 2024

weihanglo Jul 6, 2024

lschuermann commented Jul 6, 2024

lschuermann commented Jul 6, 2024

weihanglo left a comment

weihanglo commented Jul 6, 2024

weihanglo commented Jul 6, 2024

bors commented Jul 6, 2024

bors commented Jul 6, 2024

bors commented Jul 6, 2024

Allow enabling config-include feature in config #14196

Allow enabling config-include feature in config #14196

Conversation

lschuermann commented Jul 5, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jul 5, 2024

epage commented Jul 5, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jul 5, 2024

Choose a reason for hiding this comment

lschuermann Jul 6, 2024

Choose a reason for hiding this comment

weihanglo Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

lschuermann Jul 6, 2024

Choose a reason for hiding this comment

weihanglo Jul 6, 2024

Choose a reason for hiding this comment

lschuermann commented Jul 6, 2024

lschuermann commented Jul 6, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Jul 6, 2024

weihanglo commented Jul 6, 2024

bors commented Jul 6, 2024

bors commented Jul 6, 2024

bors commented Jul 6, 2024

Allow enabling `config-include` feature in config #14196

Allow enabling `config-include` feature in config #14196

lschuermann commented Jul 5, 2024 •

edited

Loading

weihanglo Jul 6, 2024 •

edited

Loading